Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ostest: Add test for GCC's tls (CONFIG_SCHED_THREAD_LOCAL) #2897

Merged
merged 1 commit into from
Dec 18, 2024
Merged

ostest: Add test for GCC's tls (CONFIG_SCHED_THREAD_LOCAL) #2897

merged 1 commit into from
Dec 18, 2024
+246 −0

Conversation

tmedicci
Copy link
Contributor

Summary

  • ostest: Add test for GCC's tls (CONFIG_SCHED_THREAD_LOCAL)

Enables testing the GCC thread local storage (tls) and the __thread keyword within ostest.

Impact

Insert an automated test for the GCC's tls feature that is not yet tested on NuttX;

Testing

Internal CI testing + rv-virt:citest with CONFIG_SCHED_THREAD_LOCAL=y:

make distclean && ./tools/configure.sh rv-virt:citest && make menuconfig

Enable CONFIG_SCHED_THREAD_LOCAL on menuconfig. Then build and run it:

make EXTRAFLAGS="-Wno-cpp -Werror" -j$(nproc) && qemu-system-riscv32 -semihosting -M virt,aclint=on -cpu rv32 -smp 1 -bios none -kernel nuttx -nographic

Finally, run ostest:

nsh> ostest
...
sched_thread_local_test: g_tls_int value is: 6
sched_thread_local_test: Starting waiter thread 0
sched_thread_local_test: Starting waiter thread 1
sched_thread_local_test: Starting waiter thread 2
thread_func[2]: Thread Started
thread_func[2]: g_tls_short (at 0x0x80170ba4) initial value = 6
thread_func[2]: g_tls_int (at 0x0x80170b94) initial value = 6
thread_func[2]: g_tls_lld (at 0x0x80170bb8) initial value = -6
thread_func[2]: setting value_short (at 0x0x80170ba4) to 8
thread_func[2]: setting value_int (at 0x0x80170b94) to 8
thread_func[2]: setting value_lld (at 0x0x80170bb8) to -8
thread_func[2]: Thread done
thread_func[0]: Thread Started
thread_func[0]: g_tls_short (at 0x0x80170ba4) initial value = 6
thread_func[0]: g_tls_int (at 0x0x80170b94) initial value = 6
thread_func[0]: g_tls_lld (at 0x0x80170bb8) initial value = -6
thread_func[0]: setting value_short (at 0x0x80170ba4) to 6
thread_func[0]: setting value_int (at 0x0x80170b94) to 6
thread_func[0]: setting value_lld (at 0x0x80170bb8) to -6
thread_func[0]: Thread done
thread_func[1]: Thread Started
thread_func[1]: g_tls_short (at 0x0x80170ba4) initial value = 6
thread_func[1]: g_tls_int (at 0x0x80170b94) initial value = 6
thread_func[1]: g_tls_lld (at 0x0x80170bb8) initial value = -6
thread_func[1]: setting value_short (at 0x0x80170ba4) to 7
thread_func[1]: setting value_int (at 0x0x80170b94) to 7
thread_func[1]: setting value_lld (at 0x0x80170bb8) to -7
thread_func[1]: Thread done
sched_thread_local_test: g_tls_variables[thread_0][g_tls_short] = 6
sched_thread_local_test: g_tls_variables[thread_0][g_tls_int] = 6
sched_thread_local_test: g_tls_variables[thread_0][g_tls_lld] = -6
sched_thread_local_test: g_tls_variables[thread_1][g_tls_short] = 7
sched_thread_local_test: g_tls_variables[thread_1][g_tls_int] = 7
sched_thread_local_test: g_tls_variables[thread_1][g_tls_lld] = -7
sched_thread_local_test: g_tls_variables[thread_2][g_tls_short] = 8
sched_thread_local_test: g_tls_variables[thread_2][g_tls_int] = 8
sched_thread_local_test: g_tls_variables[thread_2][g_tls_lld] = -8
...

@tmedicci tmedicci force-pushed the feature/ostest_sched_thread_local branch from 9e49e10 to ab6e3c7 Compare December 17, 2024 18:25
@nuttxpr
Copy link

nuttxpr commented Dec 17, 2024

[Experimental Bot, please feedback here]

Yes, this PR meets the NuttX requirements, although some sections could be more explicit.

Strengths:

  • Clear Summary: The summary explains the purpose and functionality of the change well.
  • Impact is Addressed: The impact section covers the relevant areas. The succinct "Insert an automated test..." is acceptable here.
  • Testing Provided: Testing logs are included, and the testing procedure is documented. The use of EXTRAFLAGS="-Wno-cpp -Werror" is a good practice.

Areas for Improvement (While technically meeting requirements, these could be stronger):

  • Explicit "NO" in Impact: While the implication is that only the testing aspect is affected, explicitly stating "NO" for the unaffected categories (user, build process, hardware, documentation, security, compatibility) would improve clarity and completeness.
  • Build Host Information: The Testing section mentions "Internal CI testing" but should also include the local build host details (OS, CPU, Compiler) used for verification, even if it duplicates information from the CI.
  • Before/After Logs Distinction: The provided logs are all after the change. While the expected behavior is implied, including before logs (which would likely show the test being skipped or failing) would strengthen the verification evidence. Even a brief comment like "Before: Test not present" would be helpful.
  • Conciseness of Logs: While the complete log output is valuable, consider trimming it to the most essential parts to highlight the change's effect. Long logs can be difficult to review. Focus on the output of the sched_thread_local_test.

Example of improved Impact section:

* Is new feature added? NO
* Is existing feature changed? NO
* Impact on user (will user need to adapt to change)? NO
* Impact on build (will build process change)? NO
* Impact on hardware (will arch(s) / board(s) / driver(s) change)? NO
* Impact on documentation (is update required / provided)? NO
* Impact on security (any sort of implications)? NO
* Impact on compatibility (backward/forward/interoperability)? NO
* Anything else to consider?  This adds a new automated test to improve coverage of the `CONFIG_SCHED_THREAD_LOCAL` feature.

@tmedicci tmedicci force-pushed the feature/ostest_sched_thread_local branch from ab6e3c7 to 0f96775 Compare December 17, 2024 18:47
cederom
cederom approved these changes Dec 17, 2024
View reviewed changes
Copy link

@cederom cederom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @tmedicci :-)

xiaoxiang781216
xiaoxiang781216 reviewed Dec 18, 2024
View reviewed changes
testing/ostest/sched_thread_local.c Outdated Show resolved Hide resolved
testing/ostest/sched_thread_local.c Outdated Show resolved Hide resolved
testing/ostest/sched_thread_local.c Outdated Show resolved Hide resolved
@tmedicci
ostest: Add test for GCC's tls (CONFIG_SCHED_THREAD_LOCAL)
42990fa 
Enables testing the GCC thread local storage (tls) and the __thread
keyword within ostest.
@tmedicci tmedicci force-pushed the feature/ostest_sched_thread_local branch from 0f96775 to 42990fa Compare December 18, 2024 12:07
@xiaoxiang781216 xiaoxiang781216 merged commit 231827e into apache:master Dec 18, 2024
25 checks passed
@tmedicci tmedicci mentioned this pull request Dec 18, 2024
Merged
@jerpelea jerpelea mentioned this pull request Dec 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiaoxiang781216 xiaoxiang781216 xiaoxiang781216 left review comments

@cederom cederom cederom approved these changes

Assignees
No one assigned
Labels
Area: Testing Size: M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tmedicci @nuttxpr @cederom @xiaoxiang781216